refactor(lib): extract Conversation interface #172
Conversation
|
✅ Preview binaries are ready! To test with modules: |
mafredri
changed the title
|
//cc @35C4n0r considering you're working on state serialization/restore. |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Solid refactor! Added a few questions/suggestions inline.
| mu sync.RWMutex | ||
| logger *slog.Logger | ||
| conversation *st.Conversation | ||
| conversation *st.PTYConversation |
There was a problem hiding this comment.
@johnstcn Shouldn't this be *st.Conversation ? (It's possible that I'm lacking the context/motive behind this refactor, If we have extracted an interface, shouldn't we be using it rather than coupling this to the concrete implementation ?)
There was a problem hiding this comment.
It should, but there's some more coupling to be disentangled here before we can do that.
I'm building on top of this branch. |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the screentracker package by extracting a Conversation interface and renaming the concrete implementation from Conversation to PTYConversation. The refactoring aims to provide better abstraction and support for potential alternative conversation implementations in the future.
Changes:
- Extracted a
Conversationinterface with methods:Messages(),Send(),Start(),Status(), andText() - Renamed
screentracker.Conversationtoscreentracker.PTYConversationandConversationConfigtoPTYConversationConfig - Renamed methods for brevity:
SendMessage→Send,AddSnapshot→Snapshot,StartSnapshotLoop→Start,Screen→Text - Moved and renamed
FindNewMessagefunction toscreenDiffin a newdiff.gofile with corresponding tests indiff_internal_test.go - Renamed error variables to follow Go conventions with
Errprefix (e.g.,MessageValidationErrorWhitespace→ErrMessageValidationWhitespace)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/screentracker/conversation.go | Defines the new Conversation interface and common types, removing the concrete implementation |
| lib/screentracker/pty_conversation.go | Contains the renamed PTYConversation implementation with updated method names |
| lib/screentracker/diff.go | New file containing the screenDiff function (formerly FindNewMessage) |
| lib/screentracker/diff_internal_test.go | New test file for screenDiff function |
| lib/screentracker/pty_conversation_test.go | Updated tests to use new type and method names, removed tests that were moved to diff_internal_test.go |
| lib/httpapi/server.go | Updated to use new type name PTYConversation and renamed methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i, line := range newLines[dynamicHeaderEnd+1:] { | ||
| if !oldLinesMap[line] { | ||
| firstNonMatchingLine = i | ||
| break | ||
| } | ||
| } | ||
| newSectionLines := newLines[firstNonMatchingLine:] |
There was a problem hiding this comment.
There appears to be a pre-existing bug in this logic that was moved from the original FindNewMessage function. When iterating over the sliced array newLines[dynamicHeaderEnd+1:], the index i is relative to the slice, not the full array. However, firstNonMatchingLine is then used to index into the full newLines array on line 38, which would give incorrect results.
For example, if dynamicHeaderEnd = 1 and the first non-matching line is at index 3 in the full array, the loop would set i = 1 (the index in the sliced array starting at position 2), but then newLines[1:] would start at the wrong position.
The fix should be: firstNonMatchingLine = i + dynamicHeaderEnd + 1 on line 34.
While this bug is pre-existing (moved from the original code), it could cause incorrect diff results when using the Opencode agent type with its dynamic header skipping logic.
3 participants
screentracker.Conversation->screentracker.PTYConversationConversationinterface (some methods renamed for brevity)FindLastMessagetoscreenDiff, moved todiff(_test).goalong with relevant tests.